Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wizard: fixes #1567

Merged
merged 93 commits into from
Sep 5, 2023
Merged

wizard: fixes #1567

merged 93 commits into from
Sep 5, 2023

Conversation

lucasrodes
Copy link
Member

@lucasrodes lucasrodes commented Sep 1, 2023

From #1563

  • Unselect "Generate playground notebook" and submit error: FileNotFoundError: [Errno 2] No such file or directory: '.../etl/steps/data/meadow/animal_welfare/2023-09-01/playground.ipynb'
    • ✅ Looks as if it was solved, also in master. However, Pablo reported that this issue persists in add-chick-culling-laws-data even after merging with master recently.
  • After submitting a meadow/garden step, the new files are mentioned on the new page (under "Generated files"). But I think the dag file should also be mentioned, if it has been modified.
    • ✅ Same as issue before, working on my end.
  • Field citation_producer should be expandable, given that it can be very long (as dataset description fields are).
    • ✅ Done
  • When I open wizard, I get the warning:
    2023-09-01 08:37:55.275 WARNING streamlit.runtime.caching.cache_data_api: No runtime found, using MemoryCacheStorageManager
    ['streamlit', 'run', '/Users/prosado/Documents/owid/repos/etl/apps/wizard/app.py', '--server.port', '8053', '--', '--phase', 'all', '--run-checks']
    
    • Not ideal, but this warning is expected due to the nature of how we are running streamlit under the hood.
  • When loading the resulting snapshot, I was getting a strange error. I realised that snapshot.metadata.license was None, even if license was defined in the .dvc file. It took me a while to realise that the produced .dvc file had license indented too many spaces, hence becoming an item of of origin. I think they are meant to be separate fields in the snapshot metadata. Once I removed the indent of license (and its items), the issue was fixed. So, I suppose it's a bug, and wizard should not store license inside origin (we can discuss if that should be indeed the correct structure).
    • ✅ We now export the license metadata in snapshot both as $.meta.license and $.meta.origin.license.

Additional issues:

  • Improved management of default values in input fields:
    • First option is if a value has been set to the field in this step.
    • Second option is the value from the previous step (if it was given)
    • Third option is to look in the defaults defined in wizard.utils.AppState.defaults
    • Last option is the value given by default_last when calling AppState.st_widget()
  • Change the default command from wizard to etl-wizard for consistency.

@lucasrodes lucasrodes marked this pull request as ready for review September 1, 2023 14:29
Copy link
Contributor

@pabloarosado pabloarosado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't run it, but the code looks good.

@lucasrodes lucasrodes merged commit 6ba19ae into master Sep 5, 2023
@lucasrodes lucasrodes deleted the enhance/walkthrough-2 branch September 5, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants